GH-50560: [C++][FlightRPC][ODBC] Fix SQLDescribeCol column_size/decimal_digits width - #50562
Open
vikrantpuppala wants to merge 1 commit into
Open
Conversation
|
|
github-actions
Bot
added
Component: C++
Component: FlightRPC
awaiting review
Member
I think we reserve this for things that would be a release blocker, FWIW. CC @justing-bq for review |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect SQLDescribeCol output widths in the Flight SQL ODBC driver so callers receive fully-initialized column_size for numeric/decimal types (and correct buffer-size arguments for decimal_digits), addressing GH-50560 and preventing nondeterministic garbage in upper bytes.
Changes:
- In
SQLDescribeCol, readsSQL_DESC_PRECISIONinto aSQLSMALLINTand widens intoSQLULENto avoid short writes intocolumn_size. - Corrects
decimal_digitsGetFieldbuffer sizes tosizeof(SQLSMALLINT)for scale / datetime precision paths. - Adds a new gtest file to validate
column_sizeis fully overwritten (and wires it into the ODBC test CMake target).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/arrow/flight/sql/odbc/tests/describe_col_test.cc | Adds regression tests intended to catch short writes into SQLULEN column_size and validate decimal_digits. |
| cpp/src/arrow/flight/sql/odbc/tests/CMakeLists.txt | Includes the new describe_col_test.cc in the ODBC test sources. |
| cpp/src/arrow/flight/sql/odbc/odbc_api.cc | Fixes write-width mismatch for numeric column_size and corrects buffer-size arguments for decimal_digits GetField calls. |
vikrantpuppala
force-pushed
the
fix-odbc-describecol-column-size-width
branch
from
July 21, 2026 04:21
5b1944f to
1691e5c
Compare
Author
|
@lidavidm Dropped the "Critical Fix" wording from the description |
Author
|
@justing-bq can I get a review here please? |
justing-bq
reviewed
Jul 23, 2026
justing-bq
reviewed
Jul 23, 2026
…/decimal_digits width SQLDescribeCol read SQL_DESC_PRECISION (a 2-byte SQLSMALLINT) directly into the caller's 8-byte SQLULEN* column_size output for numeric/decimal columns. The GetAttribute template writes exactly sizeof(T) bytes and ignores the claimed buffer_length, so only the low 2 bytes were written and the upper 6 bytes were left uninitialized garbage. Read the precision into a local SQLSMALLINT and widen-assign it into *column_size_ptr so all 8 bytes are written. Also fix the two decimal_digits sites (SQL_DESC_SCALE and datetime SQL_DESC_PRECISION) to pass sizeof(SQLSMALLINT) -- the true width of the SQLSMALLINT* decimal_digits output -- instead of sizeof(SQLULEN). Add describe_col_test.cc, which pre-fills column_size with an all-ones sentinel and asserts SQLDescribeCol fully overwrites it for DECIMAL and TIMESTAMP columns. Introduced by apacheGH-47724 (apache#48052). Signed-off-by: Vikrant Puppala <vikrantpuppala@gmail.com>
vikrantpuppala
force-pushed
the
fix-odbc-describecol-column-size-width
branch
from
July 29, 2026 07:04
1691e5c to
f2e0af2
Compare
alinaliBQ
reviewed
Jul 31, 2026
alinaliBQ
left a comment
alinaliBQ
left a comment
Collaborator
There was a problem hiding this comment.
nit - for the PR description, could you update to reflect that only remote tests are added?
Collaborator
|
@lidavidm could you help to approve the C++ Extra workflows? Hi @vikrantpuppala I work with @justing-bq and the PR overall LGTM. Just pending the CI checks. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
SQLDescribeColreturned uninitialized garbage in the upper bytes of itscolumn_size(ColumnSizePtr) output for numeric / decimal / integer / floatcolumns.
The numeric path read
SQL_DESC_PRECISION— stored internally as aSQLSMALLINT(2 bytes) — directly into the caller'sSQLULEN* column_size_ptr(8 bytes). The
GetAttributetemplate that ultimately performs the write copiesexactly
sizeof(T)bytes (hereT = SQLSMALLINT, so 2 bytes) and ignores theclaimed
buffer_length. The result: only the low 2 bytes of the 8-byteSQLULENwere written and the upper 6 bytes were left uninitialized. Anyapplication reading the full
column_sizesaw a wildly incorrect value.Two adjacent
decimal_digitssites (SQL_DESC_SCALEfor exact-numeric columns,SQL_DESC_PRECISIONfor datetime/interval columns) passedsizeof(SQLULEN)asthe buffer size even though
decimal_digits_ptris aSQLSMALLINT*. Those didnot corrupt memory (the write width matched the 2-byte target), but the size
argument was wrong and misleading.
Introduced by GH-47724 (#48052).
What changes are included in this PR?
column_sizepath: readSQL_DESC_PRECISIONinto a localSQLSMALLINTand widen-assign it into*column_size_ptr, so all 8 bytes ofthe
SQLULENoutput are written.decimal_digitsscale and datetime-precision paths: passsizeof(SQLSMALLINT)(the true target width) instead ofsizeof(SQLULEN).describe_col_test.ccwith parameterized (mock + remote) tests thatpre-fill
column_sizewith an all-ones sentinel, callSQLDescribeColon aDECIMALcolumn and aTIMESTAMPcolumn, and assert the full 8-bytecolumn_sizeis a sane small value (i.e. the upper bytes were overwritten),plus sane
decimal_digits. Without the fix these fail because the sentinel'supper bytes survive the short write.
Are these changes tested?
Yes — new gtest cases in
cpp/src/arrow/flight/sql/odbc/tests/describe_col_test.cc, wired into the ODBCtest
CMakeLists.txt.Are there any user-facing changes?
Yes.
SQLDescribeColnow returns a correctcolumn_sizefor numeric/decimalcolumns instead of a value with uninitialized high bytes. This is a bug fix in
externally-visible ODBC behavior; no API signatures change.